-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 replacing DeepEqual with cmp Diff in tests #8310
Conversation
Signed-off-by: DiptoChakrabarty <[email protected]> wrapper function to compare cmp diff in case panic occurs with log Signed-off-by: DiptoChakrabarty <[email protected]>
@@ -147,7 +147,7 @@ func TestSetResourceBinding(t *testing.T) { | |||
tt.resourceSetBinding.SetBinding(tt.resourceBinding) | |||
exist := false | |||
for _, b := range tt.resourceSetBinding.Resources { | |||
if reflect.DeepEqual(b.ResourceRef, tt.resourceBinding.ResourceRef) { | |||
if diff := cmp.Diff(b.ResourceRef, tt.resourceBinding.ResourceRef); diff == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if diff := cmp.Diff(b.ResourceRef, tt.resourceBinding.ResourceRef); diff == "" { | |
if cmp.Equal(b.ResourceRef, tt.resourceBinding.ResourceRef) { |
How about this?
(same in the other 2 clusterresourcesetbingins_types_test.go files)
util/conditions/compare.go
Outdated
func Check(actual interface{}, expected interface{}) (result bool, err error) { | ||
defer func() error { | ||
if recover() != nil { | ||
log.Printf("panic occured got %v expected %v", actual, expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the logs here. If the error needs to be logged it should be handled by the function using this util function.
bc78e1f
to
88d19fd
Compare
/retest |
88d19fd
to
410e461
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
51866ac
to
ee27db4
Compare
fix gci lint error
ee27db4
to
14b7314
Compare
/retest |
"github.com/google/go-cmp/cmp" | ||
) | ||
|
||
// Check compares the actual and expected values provided using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what's the motivation for this? are we unexpectedly panicking somewhere atm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enxebre I added this to catch panic cases if any occurs
it was mentioned in the issue discussion
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
what's the status of this PR? |
I'm +1 for closing this PR |
/close Feel free to reopen if you want to continue this work |
@sbueringer: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
wrapper function to compare cmp diff in case panic occurs with log
What this PR does / why we need it:
replaces reflect.DeepEqual with go-cmp diff function and also creates a wrapper function to perform comparisons that handles panics which can be used for non testing code as suggested in issue discussion. Tests for the same written
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #8186